Feat: 410 Report Verification - LFO #2714

shon-button · 2025-01-21T00:21:40Z

Addresses 410

🚀 Impact:

Model Splitting: The ReportVerification model was split to move visit-related fields (visit_name, visit_type, other_facility_name, other_facility_coordinates) into a new model, ReportVerificationVisit
New Relationship: ForeignKey relationship from ReportVerificationVisit to ReportVerification
New Constraint: CheckConstraint in ReportVerificationVisit to enforce that visit_coordinates must be provided when is_other_visit=True
Database schemas: updated to introduce the new verification_visit table

📝 To Dos:

Improve tests, see 541
Possibly refactor the RJSF schema(s) to apply all rules, unless this is not possible when applying rules based on dynamic values. See tech debt ticket for more details: 536

🔬 Local Testing:

From terminal command, start the api server:

cd bc_obps
make reset_db
make run

From terminal command, start the app development server:

cd bciers && yarn dev-all

Tests SFO "Site(s) Visited" conditional fields*

Navigate to http://localhost:3000
Click button "Log in with Business BCeID"
Navigate to Bugle SFO - Registered verification step: http://localhost:3000/reporting/reports/1/verification
Complete the form with field value Site(s) Visited= "None"
Click button Save & Continue
Expected Results

Form data is saved to the database
Attachment page is displayed

Navigate to Bugle SFO - Registered verification step: http://localhost:3000/reporting/reports/1/verification
Expected Results

Form data is displayed from the database

Complete the form with field value Site(s) Visited= "Other"
Click button Save & Continue
Expected Results

Form data is saved to the database
Attachment page is displayed

Navigate to Bugle SFO - Registered verification step: http://localhost:3000/reporting/reports/1/verification
Expected Results

Form data is displayed from the database

Complete the form with field value Site(s) Visited= "Facility 22"
Click button Save & Continue
Expected Results

Form data is saved to the database
Attachment page is displayed

Navigate to Bugle SFO - Registered verification step: http://localhost:3000/reporting/reports/1/verification
Expected Results

Form data is displayed from the database

Tests LFO "Site(s) Visited" conditional fields*

Navigate to http://localhost:3000
Click button "Log in with Business BCeID"
Navigate to Banana LFO - Registered verification step: http://localhost:3000/reporting/reports/2/verification
Complete the form with field value Site(s) Visited= "None"
Click button Save & Continue
Expected Results

Form data is saved to the database
Attachment page is displayed

Navigate to Banana LFO - Registered verification step: http://localhost:3000/reporting/reports/2/verification
Expected Results

Form data is displayed from the database

Complete the form with field value Site(s) Visited= "Other"
Click button Save & Continue
Expected Results

Form data is saved to the database
Attachment page is displayed

Navigate to ``Banana LFO - Registered verification step: http://localhost:3000/reporting/reports/2/verification
Expected Results

Form data is displayed from the database

Complete the form with field value Site(s) Visited= "Facility 1"
Click button Save & Continue
Expected Results

Form data is saved to the database
Attachment page is displayed

Navigate to Banana LFO - Registered verification step: http://localhost:3000/reporting/reports/2/verification
Expected Results

Form data is displayed from the database

pbastia

Nice work!! I've left a few comments, mostly small stuff.

I think there's some RJSF features that we can use to cut down on the schema/data manipulation, ideally we're writing code where only the form drives the data, and the components only needs to (hopefully not too much) transform it before submitting it to the API

pbastia · 2025-01-24T21:38:07Z

bc_obps/reporting/models/report_verification_visit.py

+        constraints = [
+            models.CheckConstraint(
+                name="other_facility_must_have_coordinates",
+                check=~Q(is_other_visit=True) | Q(visit_coordinates__isnull=False),


I think we want to exclude the case "is_other_visit=True and visit_coordinates__isnull=True"

check=~(Q(is_other_visit=True) & Q(visit_coordinates__isnull=True))

pbastia · 2025-01-24T21:39:07Z

bc_obps/reporting/schema/report_verification.py

+    verification_body_name: str = Field(...)
+    accredited_by: str = Field(...)
+    scope_of_verification: str = Field(...)
+    threats_to_independence: bool = Field(...)
+    verification_conclusion: str = Field(...)


these are redundant with the fields = ... in the Meta class, we can just delete these lines

pbastia · 2025-01-24T21:41:22Z

bc_obps/reporting/schema/report_verification.py

+    visit_name: str = Field(...)
+    visit_type: str = Field(choices=ReportVerificationVisit.VisitType.choices)
+    is_other_visit: bool = Field(...)
+    visit_coordinates: str = Field(required=False)


Same as above, I think the choices and required options will be inferred from the model if we just remove these lines

pbastia · 2025-01-24T21:43:12Z

bc_obps/reporting/schema/report_verification.py

+    report_verification_visits: List[ReportVerificationVisitSchema] = Field(default_factory=list) 
+
+    class Meta(BaseReportVerification.Meta):
+        fields = BaseReportVerification.Meta.fields + ['report_version']


For all these schemas we could actually generate them dynamically, since they're just a subset of the fields on the model: https://django-ninja.dev/guides/response/django-pydantic-create-schema/

pbastia · 2025-01-24T21:58:05Z

bc_obps/reporting/service/report_verification_service.py

@@ -13,17 +15,19 @@ class ReportVerificationService:
    @staticmethod
    def get_report_verification_by_version_id(
        report_version_id: int,
-    ) -> ReportVerification:
+    ) -> ReportVerificationOut:


We should consider decoupling the service layer from the API layer who owns these data structures. The service layer can declare similar types, or just return the Model types

pbastia · 2025-01-24T22:02:13Z

bc_obps/reporting/service/report_verification_service.py

+        # Fetch ReportVerificationVisit instances corresponding to the IDs
+        visit_instances_to_keep = ReportVerificationVisit.objects.filter(id__in=visit_ids_to_keep)
+
+        # Update report_verification_visits
+        report_verification.report_verification_visits.set(visit_instances_to_keep)
+


I don't think this is necessary, all the ReportVerificationVisit models have been created with the right report_verification field set, and this is not an m2m relationship.
We can remove these few lines

pbastia · 2025-01-24T22:03:22Z

bc_obps/reporting/tests/api/test_report_verification_api.py

+        # Create related ReportVerificationVisit instances and link them
+        report_verification_visits = baker.make_recipe(
+            "reporting.tests.utils.report_verification_visit",
+            _quantity=2,
+        )
+
+        # Attach the visits to the report_verification instance
+        self.report_verification.report_verification_visits.set(report_verification_visits)


Suggested change

# Create related ReportVerificationVisit instances and link them

report_verification_visits = baker.make_recipe(

"reporting.tests.utils.report_verification_visit",

_quantity=2,

)

# Attach the visits to the report_verification instance

self.report_verification.report_verification_visits.set(report_verification_visits)

# Create related ReportVerificationVisit instances and link them

report_verification_visits = baker.make_recipe(

"reporting.tests.utils.report_verification_visit",

report_verification=report_verification,

_quantity=2,

)

pbastia · 2025-01-24T22:10:35Z

bciers/apps/reporting/src/app/components/verification/VerificationForm.tsx

+      is_other_visit: true,
+      visit_coordinates: formData.visit_others.visit_coordinates || "",
+    });
+  }


I'm pretty sure there is a way to bake this in to the rjsf structure instead, with if then else or oneOf / anyOf. Happy to try to pair on that if you want!

bciers/apps/reporting/src/app/components/verification/VerificationForm.tsx

pbastia · 2025-01-24T22:17:50Z

bciers/apps/reporting/src/app/components/verification/VerificationPage.tsx

+      initialData.visit_types = undefined;
+      initialData.visit_others = [{}];
+    }
+  }


This feels like work that could be done on the server by the API layer (maybe in django ninja schema?): preparing the data for the form and the client page

pbastia

Very nice! I'm just leaving a couple of nitpicks, but this is ready to go otherwise :)

pbastia · 2025-01-31T23:20:26Z

bc_obps/reporting/schema/report_verification.py

+    visit_name: str
+    visit_type: Optional[str] = Field(None)
+    is_other_visit: bool
+    visit_coordinates: str


These are inferred from the django model, they're redundant with the Meta fields = [...] declaration. We can just remove those lines

@pbastia
FYI: when I remove the "redundant fields" I get MyPy failed

Mypy.....................................................................Failed reporting/service/report_verification_service.py:67:31: error: "ReportVerificationVisitSchema" has no attribute "visit_type" [attr-defined] reporting/service/report_verification_service.py:68:35: error: "ReportVerificationVisitSchema" has no attribute "is_other_visit" [attr-defined] reporting/service/report_verification_service.py:69:38: error: "ReportVerificationVisitSchema" has no attribute "visit_coordinates" [attr-defined] reporting/service/report_verification_service.py:74:28: error: "ReportVerificationVisitSchema" has no attribute "visit_name" [attr-defined]

pbastia · 2025-02-03T18:26:26Z

bc_obps/reporting/tests/api/test_report_verification_api.py

+        assert len(response_json["report_verification_visits"]) == len(payload.report_verification_visits)
+        for i, visit_data in enumerate(response_json["report_verification_visits"]):
+            expected_visit = payload.report_verification_visits[i]
+            print(expected_visit)


print left behind

pbastia · 2025-02-03T18:37:36Z

bciers/apps/reporting/src/app/components/verification/createVerificationUiSchema.ts

+export const createVerificationUiSchema = (
+  schemaType: "SFO" | "LFO",
+): RJSFSchema => {
+  // Determine the schema based on the schemaType
+  const localUiSchema: RJSFSchema =
+    schemaType === "SFO" ? { ...sfoUiSchema } : { ...lfoUiSchema };
+
+  // Return the customized schema.
+  return localUiSchema;
+};


Given the UiSchemas are only used in this one form, I think we can safely return the object directly

(also, the {...sfoUiSchema} syntax only allows for a shallow copy, so its usefulness is already debatable. This is why we write it as JSON.parse(JSON.stringify(stuff)) sometimes, although this will not work if we have functions in the object values)

Suggested change

export const createVerificationUiSchema = (

schemaType: "SFO" | "LFO",

): RJSFSchema => {

// Determine the schema based on the schemaType

const localUiSchema: RJSFSchema =

schemaType === "SFO" ? { ...sfoUiSchema } : { ...lfoUiSchema };

// Return the customized schema.

return localUiSchema;

};

export const getVerificationUiSchema = (

schemaType: "SFO" | "LFO",

): RJSFSchema => {

// Determine the schema based on the schemaType

const localUiSchema: RJSFSchema =

schemaType === "SFO" ? sfoUiSchema : lfoUiSchema;

// Return the customized schema.

return localUiSchema;

};

chore: update verification service chore: cleanup chore: cleanup test: verification api get chore: add verification operation type chore: ad shared verification schema properties chore: add verification dependacncies chore: shared schemas chore: refactor SFO updateReportVerificationVisits chore: cleanup chore: cleanup

chore: cleanup chore: cleanup chore: cleanup

chore: cleanup chore: cleanup chore: cleanup chore: cleanup

chore: cleanup chore: cleanup chore: cleanup chore: cleanup chore: cleanup chore: cleanup chore: cleanup chore: cleanup chore:cleanup chore: cleanup chore: revert

shon-button force-pushed the feat/410-verification-lfo branch 8 times, most recently from 1c5ce43 to 26f888f Compare January 24, 2025 20:45

pbastia reviewed Jan 24, 2025

View reviewed changes

shon-button force-pushed the feat/410-verification-lfo branch 3 times, most recently from 3040f10 to 8485ae0 Compare January 27, 2025 19:26

shon-button marked this pull request as ready for review January 27, 2025 19:27

shon-button force-pushed the feat/410-verification-lfo branch 12 times, most recently from 487640d to e405d43 Compare February 3, 2025 17:34

pbastia approved these changes Feb 3, 2025

View reviewed changes

shon-button force-pushed the feat/410-verification-lfo branch 4 times, most recently from fb25262 to d6c7c61 Compare February 3, 2025 21:02

shon-button added 6 commits February 3, 2025 16:09

feat: report verification for LFO

5cf16a4

chore: update verification RJSF schemas

4b629e8

tests: update verification api/service test

a327afe

tests: verification page

287bad2

chore: cleanup chore: cleanup chore: cleanup

chore: verification data handling functions

8393d7f

chore: cleanup chore: cleanup chore: cleanup chore: cleanup

shon-button force-pushed the feat/410-verification-lfo branch 2 times, most recently from b18eb7c to 0e30a94 Compare February 3, 2025 23:57

tests: add verification utils tests

c39bf13

chore: cleanup chore: cleanup chore: cleanup chore: cleanup chore: cleanup chore: cleanup chore: cleanup chore: cleanup chore:cleanup chore: cleanup chore: revert

shon-button force-pushed the feat/410-verification-lfo branch from 5fa3ca4 to c39bf13 Compare February 4, 2025 00:22

shon-button merged commit 7db253a into develop Feb 4, 2025
41 of 42 checks passed

shon-button deleted the feat/410-verification-lfo branch February 4, 2025 00:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: 410 Report Verification - LFO #2714

Feat: 410 Report Verification - LFO #2714

shon-button commented Jan 21, 2025 •

edited

Loading

pbastia left a comment

pbastia Jan 24, 2025

pbastia Jan 24, 2025

pbastia Jan 24, 2025

pbastia Jan 24, 2025

pbastia Jan 24, 2025

pbastia Jan 24, 2025

pbastia Jan 24, 2025

pbastia Jan 24, 2025

pbastia Jan 24, 2025

pbastia left a comment

pbastia Jan 31, 2025

shon-button Feb 3, 2025 •

edited

Loading

pbastia Feb 3, 2025

pbastia Feb 3, 2025 •

edited

Loading

Feat: 410 Report Verification - LFO #2714

Feat: 410 Report Verification - LFO #2714

Conversation

shon-button commented Jan 21, 2025 • edited Loading

🚀 Impact:

📝 To Dos:

🔬 Local Testing:

Tests SFO "Site(s) Visited" conditional fields*

Tests LFO "Site(s) Visited" conditional fields*

pbastia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbastia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shon-button Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbastia Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

shon-button commented Jan 21, 2025 •

edited

Loading

shon-button Feb 3, 2025 •

edited

Loading

pbastia Feb 3, 2025 •

edited

Loading